fix: Reject credentials on pseudo-accounts and unpin Vault/LoanBroker (FN-36) by tyalymov · Pull Request #7877 · XRPLF/rippled

tyalymov · 2026-07-27T14:48:27Z

High Level Overview of Change

CredentialCreate let any existing account be the Subject, including Vault, LoanBroker, and AMM pseudo-accounts. A pseudo-account can't sign, so it can never accept or delete a credential issued to it; the credential stays pinned in the pseudo-account's owner directory and blocks the deletion of the owning object, pinning it and its owner reserve indefinitely. This rejects such credentials and clears any that already exist, behind a new amendment.

Ref: FN-36.

Context of Change

CredentialCreate::preclaim only checked that the Subject account existed. When the subject differs from the issuer, doApply links the unaccepted credential into the subject's owner directory. For a pseudo-account subject that link is permanent: pseudo-accounts can't submit CredentialAccept or CredentialDelete, and the object owner is neither issuer nor subject, so it can't delete a non-expired credential either.

The stuck entry then blocks cleanup of the owning object:

VaultDelete and LoanBrokerDelete require the pseudo-account's owner directory to be empty and fail with tecHAS_OBLIGATIONS.
For AMM the failure is earlier and harsher: a sole-LP withdrawal runs isOnlyLiquidityProvider, which walks the AMM account's owner directory and returns tecINTERNAL on the unexpected credential entry, so the AMM can't be fully withdrawn or deleted.

Behind the new fixCredentialPseudoAccount amendment:

CredentialCreate::preclaim rejects a pseudo-account Subject (Vault / LoanBroker / AMM) with tecPSEUDO_ACCOUNT.
VaultDelete and LoanBrokerDelete delete any credentials pinned to the pseudo-account before removing it. A non-credential object still yields tecHAS_OBLIGATIONS.
isOnlyLiquidityProvider skips credential entries so an AMM with a pre-amendment pin can still be withdrawn, and deleteAMMAccount removes the pinned credentials before deleting the trust lines.

The cleanup is a shared helper credentials::deletePseudoAccountCredentials, reusing the existing cleanupOnAccountDelete walker and credentials::deleteSLE. It only removes credentials and leaves any other object in place, so unpinned deletions are unaffected. Everything is gated on the amendment, so pins created before it activates are cured and no new ones can be created after.

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

New transaction outcomes for CredentialCreate, VaultDelete, LoanBrokerDelete, and AMM withdrawal/deletion, all gated behind the new fixCredentialPseudoAccount amendment.

Test Plan

Added testCredentialPinsPseudoAccount to Vault_test, LoanBroker_test, and AMM_test. With the amendment off, each pins a credential to the pseudo-account and confirms the object can't be deleted (tecHAS_OBLIGATIONS for Vault/LoanBroker, tecINTERNAL for AMM). It then enables the amendment and confirms a new pin is rejected with tecPSEUDO_ACCOUNT and the object deletes cleanly, with the credential removed and the issuer's owner count restored. Full unit-test and ctest suites are green.

codecov · 2026-07-27T15:19:57Z

Codecov Report

❌ Patch coverage is 96.87500% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ibxrpl/tx/transactors/lending/LoanBrokerDelete.cpp	80.0%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

xrplf-ai-reviewer

Unbounded work / DoS vulnerability flagged inline.

xrplf-ai-reviewer · 2026-07-28T13:17:05Z

+    // owning object) can be deleted. Other objects are left untouched: the
+    // caller's own checks decide whether the remaining directory blocks
+    // deletion.
+    return cleanupOnAccountDelete(


DoS risk: unbounded deletion with no maxNodesToDelete limit. Unlike deleteAMMTrustLines (512 cap) and AccountDelete (1000 cap), an attacker can pin credentials to a pseudo-account and force one transaction to walk and delete an arbitrarily large directory. Add maxNodesToDelete parameter (e.g., 512), propagate tecINCOMPLETE to callers, and allow retryable cleanup instead of one-pass enumeration.

Example pattern to follow:

VaultDelete, LoanBrokerDelete, deleteAMMAccount should treat tecINCOMPLETE as retryable, allowing multiple transactions to make cleanup progress

Mirror the implementation in deleteAMMTrustLines (512 limit, returns tecINCOMPLETE on partial progress)

Good catch, clanker!

@tyalymov once maxNodesToDelete is added, let's add a test analogous to the existing kMaxDeletableAmmTrustLines overflow tests (AMM_test.cpp:5214, :7192) that pins more than the cap's worth of credentials to a pseudo-account pre-amendment and verifies tecINCOMPLETE + eventual multi-transaction cleanup.

Codecov flags one uncovered line each in AMMHelpers.cpp, LoanBrokerDelete.cpp, and VaultDelete.cpp — all the !isTesSuccess(ter) early-return branches after calling deletePseudoAccountCredentials. Once the function can return tecINCOMPLETE (per the Must Fix above), these branches become reachable and should be covered by the new test.

Thanks for catching this. I started implementing the bound and hit a snag I'd like to consult you on.

The plan was to delete up to N credentials per transaction and return tecINCOMPLETE so a follow-up transaction resumes the cleanup. That works for LoanBroker and AMM. For Vault it collides with ValidMPTIssuances: VaultDelete carries the DestroyMptIssuance privilege, and the invariant requires any applied VaultDelete to remove exactly one MPT issuance in the same transaction. An incremental VaultDelete that clears a batch of credentials and returns tecINCOMPLETE hasn't removed the share issuance yet, so it trips tecINVARIANT_FAILED.

Two directions I came up with:

Relax the invariant to require the issuance removal only on a tesSUCCESS VaultDelete, not on an in-progress tecINCOMPLETE one. It's a small change and a no-op before the amendment (VaultDelete can't return tecINCOMPLETE without it), but it does touch a consensus invariant.

Refuse the delete when the pseudo-account holds more than the cap, like AccountDelete's tefTOO_BIG, with no partial progress. Simpler, but a Vault pinned with more than the cap could then never be deleted, which doesn't fully cure the pre-amendment case.

LoanBroker and AMM I can bound incrementally regardless; it's the Vault path I'm unsure about. Any thoughts on that?

The first direction sounds right, in the transactor we first attempt to delete the credentials. If we fail to do so because there are too many of them, then the user must submit the transaction again to delete the rest of the credentials and the account.

xrplf-ai-reviewer

Unbounded credential cleanup vulnerability allowing DoS - see inline.

xrplf-ai-reviewer · 2026-07-28T14:15:04Z

    return tesSUCCESS;
 }

+TER


Unbounded credential cleanup allows DoS: attacker can pre-load credentials before amendment activates, forcing unbounded work in one transaction. Add maxNodesToDelete bound and handle tecINCOMPLETE result like deleteAMMTrustLines:

Suggested change

TER

// Define a bounding constant

static constexpr std::size_t kMaxDeletablePseudoAccountCredentials = 100;

// Modify function to accept and pass maxNodesToDelete parameter,

// returning tecINCOMPLETE if unable to complete in one transaction

TER

deletePseudoAccountCredentials(

ApplyView& view,

AccountID const& pseudoAcct,

std::size_t maxNodesToDelete,

beast::Journal j);

Tapanito

Nicely done, some minor comments.

Tapanito · 2026-07-29T08:26:45Z

+TER
+deletePseudoAccountCredentials(ApplyView& view, AccountID const& pseudoAcct, beast::Journal j)
+{
+    // Pseudo-accounts can't sign, so any credential linked into their owner


While the method name suggests thta credentials are deleted for pseudoAccounts, in reality, any accountID can be passed.

It might be a good idea to check that the account passed is a pseudo-account.

Tapanito · 2026-07-29T08:27:03Z

+// Remove every credential linked into a pseudo-account's owner directory.
+// Pseudo-accounts can't sign, so any such credential names the pseudo-account
+// as subject and can never be accepted or deleted by it. Non-credential objects
+// are left in place; the caller decides whether they block deletion.


nit: Please use doxygen style comment.

Tapanito · 2026-07-29T08:29:59Z

+// Remove every credential linked into a pseudo-account's owner directory.
+// Pseudo-accounts can't sign, so any such credential names the pseudo-account
+// as subject and can never be accepted or deleted by it. Non-credential objects
+// are left in place; the caller decides whether they block deletion.


Since after this PR CredentialCreate would not allow creating credentials for a pseudo-account, this function, at least for now, will only be useful, until the amendment that introduced it, is retired. It would be great to document why this method is needed (in case there are any pseudo-accounts already locked up).

Tapanito · 2026-07-29T08:30:52Z

 // Add new amendments to the top of this list.
 // Keep it sorted in reverse chronological order.

+XRPL_FIX    (CredentialPseudoAccount,     Supported::Yes, VoteBehavior::DefaultNo)


Each amendment requires to be voted in. Since Cleanup3_4_0 is not yet released, let's add the fix under it, rather than a new amendment.

Tapanito · 2026-07-29T08:37:04Z

+    // owning object) can be deleted. Other objects are left untouched: the
+    // caller's own checks decide whether the remaining directory blocks
+    // deletion.
+    return cleanupOnAccountDelete(


Good catch, clanker!

@tyalymov once maxNodesToDelete is added, let's add a test analogous to the existing kMaxDeletableAmmTrustLines overflow tests (AMM_test.cpp:5214, :7192) that pins more than the cap's worth of credentials to a pseudo-account pre-amendment and verifies tecINCOMPLETE + eventual multi-transaction cleanup.

Codecov flags one uncovered line each in AMMHelpers.cpp, LoanBrokerDelete.cpp, and VaultDelete.cpp — all the !isTesSuccess(ter) early-return branches after calling deletePseudoAccountCredentials. Once the function can return tecINCOMPLETE (per the Must Fix above), these branches become reachable and should be covered by the new test.

… (FN-36) CredentialCreate accepted any existing Subject, including Vault, LoanBroker, and AMM pseudo-accounts. A pseudo-account can't sign, so it can never accept or delete a credential issued to it; the unaccepted credential stays pinned in the pseudo-account's owner directory and blocks deletion of the owning object, locking it and its owner reserve indefinitely. Behind fixCleanup3_4_0: - CredentialCreate::preclaim rejects a pseudo-account Subject with tecPSEUDO_ACCOUNT. - VaultDelete and LoanBrokerDelete remove any credentials pinned to the pseudo-account before deleting it. - isOnlyLiquidityProvider ignores credential entries and deleteAMMAccount removes them, so an AMM pinned before the amendment can still be withdrawn and deleted. This clears objects pinned before the amendment and prevents new pins after it.

github-actions · 2026-07-30T13:19:32Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

depthfirst-app Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread src/libxrpl/tx/transactors/vault/VaultDelete.cpp Outdated

tyalymov requested a review from Tapanito July 28, 2026 10:49

xrplf-ai-reviewer Bot suggested changes Jul 28, 2026

View reviewed changes

Tapanito requested a review from a1q123456 July 28, 2026 13:42

xrplf-ai-reviewer Bot suggested changes Jul 28, 2026

View reviewed changes

Tapanito requested changes Jul 29, 2026

View reviewed changes

tyalymov force-pushed the FN-36-credential_pins_pseudo_account branch from 9bb06fa to ae41661 Compare July 29, 2026 10:34

tyalymov force-pushed the FN-36-credential_pins_pseudo_account branch from ae41661 to 02de7a5 Compare July 29, 2026 12:46

tyalymov requested a review from Tapanito July 29, 2026 13:04

github-actions Bot added the PR: has conflicts label Jul 30, 2026

-TER
+// Define a bounding constant
+static constexpr std::size_t kMaxDeletablePseudoAccountCredentials = 100;
+// Modify function to accept and pass maxNodesToDelete parameter,
+// returning tecINCOMPLETE if unable to complete in one transaction
+TER
+deletePseudoAccountCredentials(
+    ApplyView& view,
+    AccountID const& pseudoAcct,
+    std::size_t maxNodesToDelete,
+    beast::Journal j);

Uh oh!

Conversation

tyalymov commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

API Impact

Test Plan

Uh oh!

Uh oh!

codecov Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyalymov Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

tyalymov Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tapanito left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyalymov Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyalymov Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyalymov Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyalymov Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tyalymov commented Jul 27, 2026 •

edited

Loading

codecov Bot commented Jul 27, 2026 •

edited

Loading

tyalymov Jul 29, 2026 •

edited

Loading

tyalymov Jul 29, 2026 •

edited

Loading

tyalymov Jul 29, 2026 •

edited

Loading

tyalymov Jul 29, 2026 •

edited

Loading

tyalymov Jul 29, 2026 •

edited

Loading

tyalymov Jul 29, 2026 •

edited

Loading